-
-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[VNEXT] feat: Opt-In Anlytics #277
base: vnext
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request introduces modifications across several files to enhance analytics data collection within the application. Key updates include the addition of a new Changes
Possibly related PRs
Suggested labels
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 LanguageTooldocs/en/analytics/privacy.md[uncategorized] ~22-~22: This verb does not appear to agree with the subject. Consider using a different form. (AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT) [style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) [uncategorized] ~49-~49: A comma may be missing after the conjunctive/linking adverb ‘Currently’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA) [grammar] ~49-~49: The word “cybersecurity” is spelled as one. (CYBER_COMPOUNDS) [style] ~53-~53: ‘prior to’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_PRIOR_TO) 🪛 Markdownlint (0.35.0)docs/en/analytics/privacy.md58-58: Expected: h2; Actual: h3 (MD001, heading-increment) 49-49: null (MD034, no-bare-urls) 62-62: null (MD034, no-bare-urls) 64-64: null (MD034, no-bare-urls) 🔇 Additional comments (2)docs/en/analytics/privacy.md (2)
Define specific data retention periods and enhance compliance documentation. The current indefinite retention policy needs refinement:
Additionally, explicitly address compliance with:
🧰 Tools🪛 LanguageTool[style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
The current list of collected data points should be exhaustive to ensure transparency. Please clarify:
Consider implementing a versioned schema for analytics data to track changes in collection scope over time. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying homebox-docs with Cloudflare Pages
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Are the analytics going to be publicly viewable? |
I think we could make the analytics public, we may have to take a deeper look at it though to see what kind of impact (if any) that might have on server load. I'm doubtful it would increase the load in any significant way, so a public analytics dashboard would be likely feasible. And I think makes sense. |
The public analytics view has been added to the docs in the most recent commits. Still need to fix the PR CI stuff, and then this will probably be good to go. (Once we get the privacy policy sorted) |
Probably worth tracking if running in docker and version of homebox. |
Appreciate this being opt-in! Looks like pretty basic data so I'll be happy to enable, but glad to have the option. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
docs/en/analytics/privacy.md (1)
54-60
: Fix document formatting issues.Several formatting issues need attention:
Apply these changes:
-### Contact Us +## Contact Us -* Discord: https://discord.homebox.software/ -* Reddit Modmail: r/Homebox -* Github: https://git.homebox.software/ +* Discord: [Join our Discord](https://discord.homebox.software/) +* Reddit Modmail: [r/Homebox](https://reddit.com/r/Homebox) +* Github: [Homebox Repository](https://git.homebox.software/)🧰 Tools
🪛 Markdownlint
54-54: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
58-58: null
Bare URL used(MD034, no-bare-urls)
60-60: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- docs/en/analytics/privacy.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/en/analytics/privacy.md
[style] ~45-~45: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... their data contributes to Homebox. * Right to withdraw consent: Users may opt ou...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~49-~49: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...or Reddit) at a minimum 7 full days prior to any change being conducted (unless mand...(EN_WORDINESS_PREMIUM_PRIOR_TO)
🪛 Markdownlint
docs/en/analytics/privacy.md
54-54: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
58-58: null
Bare URL used(MD034, no-bare-urls)
60-60: null
Bare URL used(MD034, no-bare-urls)
🔇 Additional comments (1)
docs/en/analytics/privacy.md (1)
9-18
:⚠️ Potential issueConsider adding data transmission security details.
While the data collection points are well-defined, the policy should explicitly state how data is encrypted during transmission to ensure secure data transfer to the Plausible Analytics instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🛑 Comments failed to post (1)
backend/app/api/main.go (1)
115-137: 💡 Codebase verification
⚠️ Potential issue🔒 Security improvements needed in analytics HTTP request handling
The current implementation has several security concerns that should be addressed:
- Response body handling is not following best practices:
-err = res.Body.Close() -if err != nil { - log.Error().Err(err).Msg("failed to send analytics request") -} +if res != nil { + defer res.Body.Close() + // Read and discard body to prevent resource leaks + _, _ = io.Copy(io.Discard, res.Body) + if res.StatusCode >= 400 { + log.Error().Int("status", res.StatusCode).Msg("analytics request failed") + } +}
- HTTP client should be reused instead of creating a new one for each request:
+var analyticsClient = &http.Client{ + Timeout: 10 * time.Second, +} + func sendAnalytics() { - client := &http.Client{ - Timeout: 10 * time.Second, - } - res, err := client.Do(req) + res, err := analyticsClient.Do(req)
- Error handling should be more robust and avoid potential nil pointer dereference:
if err != nil { - log.Error().Err(err).Msg("failed to send analytics request") + log.Error().Msg("analytics request failed: network or timeout error") + return }These changes will improve security, resource management, and error handling robustness.
🔗 Analysis chain
Enhance security measures in analytics HTTP request handling.
While the implementation uses HTTPS and includes basic error handling, several security improvements are recommended:
- Add certificate verification settings:
client := &http.Client{ Timeout: 10 * time.Second, + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + }, + }, }
- Improve error handling to prevent information leakage:
-if err != nil { - log.Error().Err(err).Msg("failed to send analytics request") +if err != nil { + log.Error().Msg("analytics request failed") }
- Add proper response body handling:
-err = res.Body.Close() +if res != nil { + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + log.Error().Msg("unexpected status from analytics endpoint") + } +}Also consider:
- Implementing retry mechanism with exponential backoff
- Adding request context with timeout
- Implementing rate limiting
Let me analyze the HTTP client implementation more thoroughly to provide accurate security recommendations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper TLS configuration in the codebase rg -A 5 "tls.Config"Length of output: 230
Script:
#!/bin/bash # Search for HTTP client configurations and TLS settings rg -A 10 "http.Client" # Search for any existing TLS imports rg "crypto/tls" # Look for any transport configurations rg "http.Transport" # Check for response body handling patterns rg -A 5 "Body.Close"Length of output: 888
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🛑 Comments failed to post (2)
docs/en/analytics/privacy.md (2)
20-32:
⚠️ Potential issueEnhance security measures documentation.
While self-hosting Plausible provides control over data, the policy should address:
- Access controls and authentication for the Plausible instance
- Data encryption at rest and in transit
- Regular security audits and monitoring
- Incident response procedures
Consider adding a section detailing these security aspects to provide transparency about data protection measures.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...a collected through Homebox's analytics are managed and stored in our self-hosted P...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
47-64: 🛠️ Refactor suggestion
Strengthen third-party security requirements and change management.
Third-party security:
- Document security requirements for third-party services
- Specify data processing agreements
- Regular security assessment procedures
Change management:
- Consider adding in-app notifications for policy changes
- Implement version control for privacy policy
- Document change approval process
Fix the bare URLs and heading structure:
-* Discord: https://discord.homebox.software/ -* Reddit Modmail: r/Homebox -* Github: https://git.homebox.software/ +* [Discord](https://discord.homebox.software/) +* [Reddit Modmail](https://www.reddit.com/r/Homebox) +* [Github](https://git.homebox.software/) -### Contact Us +## Contact Us📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# 6. 3rd Parties Homebox may use 3rd parties as part of providing the web services to operate. Currently this includes only Cloudflare, who handles cyber security services to our analytics endpoints and websites. You can view their privacy policy at https://www.cloudflare.com/privacypolicy/ # 7. Policy Changes Any changes to this privacy policy will be communicated to users through Homebox's update channels (namely Discord or Reddit) **at a minimum** 7 full days prior to any change being conducted (unless mandated by law to do so otherwise). Continued use of Homebox following updates will imply acceptance of the revised policy, and users are free to opt-out of analytics at any point without impact to their usage of Homebox software. ## Contact Us For any questions about this privacy policy or your data, please contact the team through our official channels: * [Discord](https://discord.homebox.software/) * [Reddit Modmail](https://www.reddit.com/r/Homebox) * [Github](https://git.homebox.software/)
🧰 Tools
🪛 LanguageTool
[uncategorized] ~49-~49: A comma may be missing after the conjunctive/linking adverb ‘Currently’.
Context: ... providing the web services to operate. Currently this includes only Cloudflare, who hand...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[grammar] ~49-~49: The word “cybersecurity” is spelled as one.
Context: ...s includes only Cloudflare, who handles cyber security services to our analytics endpoints and...(CYBER_COMPOUNDS)
[style] ~53-~53: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...or Reddit) at a minimum 7 full days prior to any change being conducted (unless mand...(EN_WORDINESS_PREMIUM_PRIOR_TO)
🪛 Markdownlint (0.35.0)
58-58: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
49-49: null
Bare URL used(MD034, no-bare-urls)
62-62: null
Bare URL used(MD034, no-bare-urls)
64-64: null
Bare URL used(MD034, no-bare-urls)
What type of PR is this?
What this PR does / why we need it:
This PR is adding some basic analytics to help us understand what kind of systems Homebox is running on so we can better optimize and understand it's use. At no point is any sensitive private information shared with us, and we are sending the data to our own self-hosted Plausible Analytics instance.
We do not store Region/City level location data, but we may store information regarding the country in which instances are sending data from (again, never anything identifiable).
This analytics data is currently only sent on application startup.
Summary by CodeRabbit
Release Notes
New Features
HBOX_ALLOW_ANALYTICS
, to control analytics settings.Documentation Updates
HBOX_ALLOW_ANALYTICS
in Docker configurations.Chores